-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add BinaryContent.from_path convenience method
#3482
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| if media_type is None: | ||
| media_type = 'application/octet-stream' | ||
|
|
||
| binary_content = BinaryContent(data=path.read_bytes(), media_type=media_type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not on BinaryContent? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean the default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dsfaccini I meant that read_path was intended to be a method on BinaryContent, not FilePart, so I'm curious why you went the other way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
damn, that wasn't an active decision, honestly I just mixed up which class the method was supposed to go on, and I think my brain just though it made more sense on the FilePart (i.e. FilePart.from_path instead of BinaryContent.from_path)
I'll fix it now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is fixed now
FilePart.from_pathBinaryContent.from_path convenience method
| if media_type is None: | ||
| media_type = 'application/octet-stream' | ||
|
|
||
| return cls(data=path.read_bytes(), media_type=media_type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use narrow_type here to get an BinaryImage in case it's an image
| path = Path(path) | ||
| if not path.exists(): | ||
| raise FileNotFoundError(f'File not found: {path}') | ||
| media_type, _ = guess_type(path.as_posix()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we need the as_posix for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought I had a linter error from the pathlib.Path type, probably was something else
tests/test_messages.py
Outdated
| ) | ||
|
|
||
|
|
||
| def test_file_part_from_path(tmp_path: Path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| def test_file_part_from_path(tmp_path: Path): | |
| def test_binary_content_from_path(tmp_path: Path): |
Co-authored-by: Douwe Maan <[email protected]>
Replaces #3154